[GLUTEN-8232][VL] Allow to enable dynamic openssl link in VCPKG packaging#11444
Conversation
ce8f244 to
b55532a
Compare
9febdcd to
5079457
Compare
philo-he
left a comment
There was a problem hiding this comment.
Two comments. Please check if they make sense. Thanks.
| run: | | ||
| docker pull apache/gluten:vcpkg-centos-7 | ||
| docker run -v $GITHUB_WORKSPACE:/work -w /work apache/gluten:vcpkg-centos-7 bash -c " | ||
| docker pull apache/gluten:vcpkg-centos-9 |
There was a problem hiding this comment.
Suggest setting container field with centos 9 image, which is a standard way. I assume it's compatible with GHA checkout.
| endif() | ||
|
|
||
| if(PORT STREQUAL "openssl") | ||
| set(VCPKG_LIBRARY_LINKAGE dynamic) |
There was a problem hiding this comment.
Can we use dynamic link only when FIPS is enabled?
My understanding is that FIPS is not a requirement for many users. Then, the current static link for openssl may be good for them.
There was a problem hiding this comment.
only enable this when VCPKG_DYNAMIC_OPENSSL is on
aabd20e to
2f47e7d
Compare
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks. |
|
FYI. From what I recall based on my previous investigation, the preferred approach is to dynamically link OpenSSL installed in user's environment to use the FIPS feature, rather than going through vcpkg. |
I see. Since we didn't specify an absolute path for link, the difference here is only the openssl version liked to Gluten. FIPS is transparent at link time. |
Signed-off-by: Yuan <yuanzhou@apache.org> fix Signed-off-by: Yuan <yuanzhou@apache.org> install libcrypto/libssl Signed-off-by: Yuan <yuanzhou@apache.org> fix Signed-off-by: Yuan <yuanzhou@apache.org>
73d297a to
44b6235
Compare
Signed-off-by: Yuan <yuanzhou@apache.org>
Signed-off-by: Yuan <yuanzhou@apache.org>
eb17800 to
30980cd
Compare
This reverts commit 30980cd.
Signed-off-by: Yuan <yuanzhou@apache.org>
Signed-off-by: Yuan <yuanzhou@apache.org>
This reverts commit baf5225.
|
with this feature the libvelox.so will link dynamically |
|
@zhouyuan, thanks for the update. Not sure if my understanding is correct — I'd appreciate any clarification. I've been trying to understand the practical usage of FIPS-enabled OpenSSL. It seems that in a production environment, the application should link against the OS-provided, FIPS-enabled and certified OpenSSL shared library. If so, are we enabling FIPS in vcpkg primarily for development verification? The certification process is complex and isn't designed to be repeated for every build. From what I've gathered: Does this align with the intended approach? |
Just to confirm – When using vcpkg to build shared OpenSSL libs, aren't those libs installed under the vcpkg installation directory? |
|
@philo-he In Gluten we only need to enable dynamic linking with openssl when packaging with vcpkg, which is not available in current impl. This patch enabled this feature. Note this is not enabled by default, so all community release won't be impacted
The vender need to provide a
Right the example here is to demonstrate the different linking at runtime |
There was a problem hiding this comment.
LGTM. Thanks. cc @FelixYBW
Not sure if users will prefer linking against OS-installed, certified OpenSSL libs at build time. If so, we could allow bypassing the vcpkg OpenSSL build and let CMake find the system libs instead. Since this isn't determined yet, we can do the follow-up if needed in the future.
Could you document this clearly in a separate PR for users to reference? Perhaps, we can include the following clarifications. Thank you.
- At runtime,
LD_LIBRARY_PATHshould point to the OS-provided OpenSSL package, which includeslibssl.so,libcrypto.so, and the FIPS-certifiedfips.so. - Users should ensure compatibility between the OpenSSL libraries (
libssl.soandlibcrypto.so) used at link time and those available at runtime. We recommend using the same OpenSSL version for both to avoid potential issues.
@philo-he, you are right. Ideally, we should link to libssl in the target system. We have below choices:
If 2 or 5 happens, developer either need to update the version in vcpkg/dev system or the one on target system. We just think openssl is stable enough, linking with vcpkg is much easier than system one. Let's try this solution first, and switch to yours once we encounter issues. |
|
@zhouyuan can you add the config to https://github.com/apache/gluten/blob/main/docs/get-started/build-guide.md? Also list the openssl version in vcpkg? |
What changes are proposed in this pull request?
This patch enabled dynamic openssl to include FIPS feature in vcpkg build.
The feature is off by default, it can be enabled by set env
VCPKG_DYNAMIC_OPENSSLtoONIn the runtime user will need to ensure the libssl.so & libcrypt.so are available otherwise Gluten will fail to run
How was this patch tested?
pass GHA
Related issue: #8232